Selection: moved caching related functionality to separate classes#185
Selection: moved caching related functionality to separate classes#185dg merged 1 commit intonette:masterfrom foxycode:cache
Conversation
|
Thanks for pull request. If I understand it, this is a pure refactoring that separates the code into new classes but does not change the functionality? |
|
@dg Yes, exactly :) |
There was a problem hiding this comment.
Because it is targeted to 7.1, you can use return type void.
There was a problem hiding this comment.
Please don't use property name in @param. I know that PhpStorm appends it automatically, but better is to be consistent with rest of code.
Btw is it really mixed or array?
There was a problem hiding this comment.
This is just move from original class. I wasn't sure so I reather left it as is.
There was a problem hiding this comment.
There are more variables that use null and bool to distinguish some states. I would like to fix this in second run.
There was a problem hiding this comment.
It seems that array_keys() can return only array, or not?
There was a problem hiding this comment.
Yes, and it passes tests, fixed.
There was a problem hiding this comment.
Here iare stored Selection instances.
|
Rest fixed |
There was a problem hiding this comment.
$previousAccessedColumns has typehint array and here is mixed
There was a problem hiding this comment.
My bad. I copied properties as they were. Fixed on all properties.
There was a problem hiding this comment.
$accessedColumns has typehint array and here is mixed
There was a problem hiding this comment.
$accessedColumns has typehint array and here is mixed
There was a problem hiding this comment.
$observeCache has typehint bool and here is mixed
There was a problem hiding this comment.
Fixed, allowed only Selection.
|
@dg Anything else I can do so you merge it? |
There was a problem hiding this comment.
What means mixed of? Is it array or not?
There was a problem hiding this comment.
There was lot of mess arround mixed properties, so I get rid of them and simplify code in separate commit. Let me know what you think.
There was a problem hiding this comment.
It seems according to the code that it is array|null, or not? For getReferenced too.
There was a problem hiding this comment.
Yes, you are right
There was a problem hiding this comment.
Just such a stupid thing, maybe positive condition can be used here and in setAccessedColumns
if ($this->cache) {
$this->accessedColumns[$key] = $value;
}
```There was a problem hiding this comment.
Fixed on both places.
|
Now it look much better, thanks! |
There was a problem hiding this comment.
Sry, now I realized that it can be written native as &getReferencing(string $generalCacheKey): ?array
There was a problem hiding this comment.
Yes, I should realize that too. Fixed
|
Do you want to have two commits, one with mixed and the other without mixed properties? |
|
Thanks, great! |
2 participants
@dg Cache implementation is a long term problem in
NDBT. While this is definitely not perfect, it's a starting point from which I would like to continue makingNDBTcode more clear, which wil help in future development. I spent two days with this, so I hope you appreciate it. Also, if you agree, I can maintain this repo as we spoken on NettCamp. There is lot for review and merge.